Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for matchers NonExhaustiveMatchError error and new failure matcher #6

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

andriimosin
Copy link

@andriimosin andriimosin commented Jun 17, 2017

I tried to fix few issues I faced with.

First of all, I added extra check for result.policy.default in unauthenticated matcher, cause it failed in cases when there was no policy at all. Actually, I'm not sure about it, because when you are not unauthenticated, you will not get policy to be initialized. Looks like we need more tests here, but since everything failed because of that, this solution is at least something.

I added failure matcher, because it is possible to have operation without any contracts or policies. In this case, the invalid matcher will not match, because it's require contract to exist. If you have different opinion about it, I'd like to discuss it, but in my case it helped me.

The most interesting part is NonExhaustiveMatchError. Currently, it will raise the error if don't have ALL handlers in your controller. I'm pretty sure that this solution is not final, but it will help to move forward.

All specs except one are passing. The generic handler because user handler doesn't match. test is currently failed, because in current realisation matcher will be called with user block if it was pass. But this test expecting generic handlers to be fired if user's handler doesn't match. I think it would be nice to find a solution how return to generic handlers in future, but looks like this case never worked.

I made a lot of changes here only because build was completely broken, so I was in need to update dependencies and change test_helper little bit.

I would like to discuss the future of this gem and all this changes here 😉

P.S. looks like there are fixes for this 3 issues in this PR: #4, #3, #2

…tiveMatchError in Endpoint, fixed broken specs, updated dependencies in Gemfile, minor changes
@apotonick
Copy link
Member

@andriymosin !!! 😍 🍻

I will go through your changes, but regarding the future, I am considering removing the "pattern matching" with a simple switch statement, I was going through the code with Jose Valim and he inspired this change!!! 😂

@andriimosin
Copy link
Author

Hey bro!!! Nice to hear you, at least in such form of conversation 🤣🤣🤣
Thank you, I'd like to participate in the development, cause I'm feeling really exciting about all these TB features. I have few ideas about how to make it without the pattern matching, will try to implement it in the next couple of days.
Cheers!!! 🍻🍻🍻

@apotonick
Copy link
Member

apotonick commented Jun 19, 2017

That would be amazing. The idea here is very simple (as you might have noticed already): The Result object is the only way to communicate with the outer world, or the Endpoint. The endpoint knows certain states on the result object and from there interprets what action to take (e.g. policy error will lead to a 401 with a potential custom error message in the JSON, etc. etc.).

The "knowing" of what has happened must be encoded in some kind of configurable/extendable form. ATM, I use pattern matching because I wanted to play with it, be we can simply use if here and have some hash that contains condition/action tuples in a certain order. This is where you could come and do your magic.

BTW, in TRB 2.1, you can even have different ends and can infer state/outcome from there. I will give you an update once it's presentable.

Thanks and cheers brother 🎇

@zoggxxx
Copy link

zoggxxx commented Jun 28, 2017

@andriymosin Thanks for doing this! I've actually been playing around with Endpoints all day, along with pattern matching. From what I had been reading, I was aware that this was sort of on the backburner (and not due to lack of importance), as I had been running into similar issues that you had experienced. In particular:

"The most interesting part is NonExhaustiveMatchError. Currently, it will raise the error if don't have ALL handlers in your controller. I'm pretty sure that this solution is not final, but it will help to move forward."

I noticed that too. Anyway, I can certainly get by for now...I just had my heart set on one line per controller action, not two...I guess I'll just have to live with it for now. I've learned that every time I build some utility to slightly improve pieces of my app, TRB will come up with a better solution in another week or two. I can wait (hint hint, Nick ;)).

@andriimosin
Copy link
Author

@zoggxxx you right, NonExhaustiveMatchError is a noisy piece and I'm working on removing the whole 'pattern matching' at all, as @apotonick suggested before. I hope to present something soon 😉

@zoggxxx
Copy link

zoggxxx commented Jun 30, 2017

Awesome, thanks again :)

@rpbaltazar
Copy link

This has been a bit stalled but I discussed with @apotonick earlier this year about it.
I like the idea and the more it is in production the more repetitive code we have. I finally took some time and decided to refactor the endpoint. I have not yet merged it in my endpoint repo as it was easier for me test different use cases with a real application.

https://gist.github.com/rpbaltazar/806773e9c0a12e9fbd30e5c9d27a5243

Please guys, take a look and see if this is a usable approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants